Skip to content

add device kwarg to astype API#665

Merged
kgryte merged 7 commits intodata-apis:mainfrom
steff456:astype_device
Sep 19, 2023
Merged

add device kwarg to astype API#665
kgryte merged 7 commits intodata-apis:mainfrom
steff456:astype_device

Conversation

@steff456
Copy link
Member

@steff456 steff456 commented Jul 26, 2023

This PR,

  • Adds device kwarg to astype API in the DRAFT version

It fixes #647 (comment)

@steff456 steff456 added the API change Changes to existing functions or objects in the API. label Jul 26, 2023
@steff456 steff456 requested review from kgryte and rgommers July 26, 2023 18:21
@steff456 steff456 self-assigned this Jul 26, 2023
kgryte
kgryte reviewed Jul 27, 2023
View reviewed changes
@steff456 @kgryte
Add review changes
caca863 
Co-authored-by: Athan <kgryte@gmail.com>
kgryte
kgryte approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kgryte kgryte added the topic: Device Handling Device handling. label Jul 27, 2023
@kgryte kgryte added this to the v2023 milestone Jul 27, 2023
@rgommers rgommers mentioned this pull request Jul 28, 2023
Closed
@leofang leofang requested a review from oleksandr-pavlyk July 31, 2023 04:30
leofang
leofang approved these changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@leofang leofang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too. One possible nit: IIRC we discussed about raising exceptions for casting to unsupported dtypes by a certain device. Do we want to address it in this PR, or another?

I'd like to have @oleksandr-pavlyk to take a look if possible.

@kgryte
Copy link
Contributor

kgryte commented Jul 31, 2023

@leofang Ah, you're right. We do need to address the condition where the requested device does not support the desired dtype. Although, this applies more generally to all creation functions supporting the dtype kwarg. In which case, maybe we do want to address in a follow-up PR which covers all affected APIs?

@leofang
Copy link
Contributor

leofang commented Jul 31, 2023

Yup sounds good

@ogrisel
Copy link

ogrisel commented Aug 23, 2023

Thanks for the PR, LGTM as well.

kgryte
kgryte approved these changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, @steff456!

@kgryte
Copy link
Contributor

kgryte commented Sep 19, 2023

As this PR has three approvals, will go ahead and merge.

We'll submit a follow-up PR to add guidance concerning what should happen when a device does not support an array data type when using astype and various creation functions.

@kgryte kgryte merged commit d39fc66 into data-apis:main Sep 19, 2023
@kgryte kgryte deleted the astype_device branch September 19, 2023 22:29
@ndgrigorian ndgrigorian mentioned this pull request Sep 22, 2023
Closed
@Micky774 Micky774 mentioned this pull request Mar 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kgryte kgryte kgryte approved these changes

@leofang leofang leofang approved these changes

@rgommers rgommers Awaiting requested review from rgommers

@oleksandr-pavlyk oleksandr-pavlyk Awaiting requested review from oleksandr-pavlyk

Assignees

@steff456 steff456

Labels

API change Changes to existing functions or objects in the API. topic: Device Handling Device handling.

Projects

None yet

Milestone

v2023

Development

Successfully merging this pull request may close these issues.

Add dtype keyword to to_device

4 participants

@steff456 @kgryte @leofang @ogrisel